Feat: Demo changes for FullSend Skill Testing - #370
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Advanced Run ID: Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Sushant2504 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/test ci/prow/coverage |
|
/test all |
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 6:18 AM UTC · Completed 6:32 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.08 |
|
Risk Assessment: moderate (2/5) DetailsPR expanded from 1 to 21 files with large blast radius driving change-size up, but all other signals are benign: no security-sensitive or CI changes, no dependency updates, uniformly low git churn and contention (Tier 2=1.0), and additive nature of changes; weighted composite rounds to 2, maintaining moderate level but for qualitatively different reasons than the prior single-file assessment. Previous runRisk Assessment: moderate (2/5) DetailsLow-risk change: a single new test file (119 lines) with no protected paths, no security-sensitive files, no dependency or CI changes, and a 100% test file ratio; moderate baseline for new file with no git history. |
ReviewFindingsMedium
Low
Next steps:
Previous runReviewFindingsCritical
High
Medium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The ocm-agent-operator creates a ServiceMonitor for Prometheus to scrape port 8383 metrics, but the existing NetworkPolicies only allow Alertmanager and MUO ingress. This causes TargetDown alerts because prometheus-k8s pods from openshift-monitoring cannot reach the ocm-agent metrics endpoint. Add a new NetworkPolicy that allows prometheus-k8s pods (selected by app.kubernetes.io/name=prometheus) from openshift-monitoring to reach the ocm-agent. The policy is created in both fleet and non-fleet modes since Prometheus scraping is needed in both. Uses the existing dispatch-key pattern (like NamespaceRHOBS) so NamespacePrometheus maps back to the real openshift-monitoring namespace via callerNamespace(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rset ROSAENG-62697: align ManagedNotification severity with OCM Service Log's new HCC-aligned labels
|
/fs-review |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #370 +/- ##
==========================================
+ Coverage 65.39% 66.07% +0.68%
==========================================
Files 23 23
Lines 1598 1636 +38
==========================================
+ Hits 1045 1081 +36
- Misses 473 475 +2
Partials 80 80
🚀 New features to boost your workflow:
|
|
/fs-review |
1 similar comment
|
/fs-review |
|
@Sushant2504: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 11:08 PM UTC · Completed 11:29 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.72 |
| return ns, nil | ||
| } | ||
|
|
||
| // ValidateNamespace checks if a namespace name follows Kubernetes naming conventions |
There was a problem hiding this comment.
[medium] speculative-code
ValidateNamespace() and IsSystemNamespace() are defined and tested but have zero callers anywhere in the codebase. CLAUDE.md explicitly prohibits speculative additions.
Suggested fix: Remove both functions and their tests. Reintroduce in a future PR when a caller exists.
| var namespaces []string | ||
| if ocmAgent.Spec.FleetMode { | ||
| namespaces = append(namespaces, oah.NamespaceMonitorng, oah.NamespaceRHOBS, oah.NamespaceOBO) | ||
| namespaces = append(namespaces, oah.NamespaceMonitorng, oah.NamespaceRHOBS, oah.NamespaceOBO, oah.NamespacePrometheus) |
There was a problem hiding this comment.
[medium] network-policy-scope
The new Prometheus NetworkPolicy allows ingress from Prometheus pods to all ports on OCM Agent pods. The NamespacePrometheus doc comment states the purpose is metrics scraping on port 8383, but the NetworkPolicyIngressRule has no Ports restriction. This follows the existing pattern but incrementally widens attack surface.
Suggested fix: Add a Ports field to the NetworkPolicyIngressRule restricting ingress to port 8383 (OCMAgentMetricsPort).
| // Request object not found, could have been deleted after reconcile request. | ||
| // Owned objects are automatically garbage collected. For additional cleanup logic use finalizers. | ||
| // Return and don't requeue | ||
| reqLogger.V(2).Info("OCMAgent resource not found, marking as absent") |
There was a problem hiding this comment.
[low] scope-creep
The V(2) debug logging additions are orthogonal to the other changes in this PR and should be tracked separately.
| . "github.com/onsi/gomega" | ||
| ) | ||
|
|
||
| func TestNamespace(t *testing.T) { |
There was a problem hiding this comment.
[low] code-organization
The Ginkgo test bootstrap (TestNamespace + RunSpecs) is inlined in the spec file. The repo convention places this in a separate *_suite_test.go file.
Suggested fix: Extract to pkg/util/namespace/namespace_suite_test.go.
| } | ||
|
|
||
| // ValidateNamespace checks if a namespace name follows Kubernetes naming conventions | ||
| func ValidateNamespace(ns string) error { |
There was a problem hiding this comment.
[low] logic-error
ValidateNamespace does not enforce that the first character is alphabetic. While Kubernetes relaxes RFC 1123 to allow leading digits, the function documentation claims to follow Kubernetes naming conventions without noting this edge case. The function is currently unused.
| command -v node >/dev/null || { echo "warning: node not found" >&2; emit_empty; } | ||
|
|
||
| # Get current PR head commit to filter stale comments. | ||
| head_sha=$(gh api "repos/${REPO}/pulls/${PR}" --jq '.head.sha' 2>/dev/null || true) |
There was a problem hiding this comment.
[low] shell-injection
The $PR variable is interpolated into gh api URL paths without numeric validation. Mitigated by the sandbox environment and gh CLI URL encoding.
Suggested fix: Add validation: [[ "$PR" =~ ^[0-9]+$ ]] || die "PR_NUMBER must be numeric".
| })); | ||
|
|
||
| console.log(JSON.stringify([...review, ...issue])); | ||
| " "$BOT" "$head_sha" "$review_raw" "$issue_raw" |
There was a problem hiding this comment.
[low] argument-injection
GitHub API response bodies containing untrusted comment text are passed as CLI arguments to node -e. For PRs with many comments, this could exceed ARG_MAX.
Suggested fix: Pipe JSON through stdin instead of command-line arguments.
| SeverityWarning NotificationSeverity = "Warning" | ||
| SeverityInfo NotificationSeverity = "Info" | ||
| SeverityMajor NotificationSeverity = "Major" | ||
| SeverityWarning NotificationSeverity = "Warning" // Deprecated: use SeverityModerate |
There was a problem hiding this comment.
[low] deprecation-contract
Deprecation of SeverityWarning, SeverityInfo, and SeverityMajor is communicated only via Go source comments. YAML consumers creating CRs have no visibility into the deprecation.
5 participants
Summary
This PR adds utility functions for Kubernetes namespace validation and classification, along with enhanced logging in the OCMAgent controller for better observability. These changes demonstrate typical code quality improvements and feature additions for PR review tool testing.
Changes
New Utilities (pkg/util/namespace/)
Test Coverage
Controller Improvements (controllers/ocmagent/)
Test Results
✅ All 17 tests passing
✅ go build successful
✅ go mod tidy clean
Type of Change
Checklist